scripts/performance/utils/evaluate.py (2)

684-689: Duplicated window calculation logic.

The start/end calculation at lines 684-687 duplicates the logic from validate_performance (lines 328-331). Consider extracting a helper function to ensure consistent behavior and reduce maintenance burden.

♻️ Proposed helper function extraction

Add a helper function at module level:

def _get_eval_window(config: Dict[str, Any], num_steps: int) -> tuple[int, int | None]:
    """Compute (start, end) indices for the evaluation window.
    
    Args:
        config: Performance config dict with optional eval_time_start_step,
                eval_time_end_step, and skip_first_percent_time keys.
        num_steps: Total number of steps.
    
    Returns:
        Tuple of (start_index, end_index) where end_index may be None.
    """
    start = config.get("eval_time_start_step")
    if start is None:
        start = max(1, int(num_steps * config.get("skip_first_percent_time", 0.1)))
    end = config.get("eval_time_end_step")
    return start, end

Then use it in both locations:

-    start = config.get("eval_time_start_step")
-    if start is None:
-        start = max(1, int(len(steps) * config["skip_first_percent_time"]))
-    end = config.get("eval_time_end_step")
+    start, end = _get_eval_window(config, len(steps))
     current_stable = current_gpu_util_values[start:end]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/performance/utils/evaluate.py` around lines 684 - 689, Extract the
duplicated start/end window logic into a module-level helper function (e.g.,
_get_eval_window(config: Dict[str, Any], num_steps: int) -> tuple[int,
Optional[int]]) that implements the same behavior as the duplicated blocks (use
config.get("eval_time_start_step"), fallback to max(1, int(num_steps *
config.get("skip_first_percent_time", 0.1))), and return eval_time_end_step as
end or None); then replace the duplicated logic in evaluate.py (the block
computing start/end before computing current_avg_iter_time_ms and
golden_avg_iter_time_ms) and the logic inside validate_performance with calls to
_get_eval_window(performance_config, len(steps)) to ensure consistent behavior.

---

328-333: Consider validating start and end bounds.

The current implementation doesn't validate that:

1. start and end are non-negative (negative indices have different Python slice semantics)
2. start < end (empty slice would cause np.nanmean to return nan)

While unlikely in practice, invalid CLI inputs could produce confusing behavior.

🛡️ Optional: Add bounds validation

     start = config.get("eval_time_start_step")
     if start is None:
         start = max(1, int(len(steps) * config["skip_first_percent_time"]))
+    elif start < 0:
+        raise ValueError(f"eval_time_start_step must be non-negative, got {start}")
     end = config.get("eval_time_end_step")
+    if end is not None and end < 0:
+        raise ValueError(f"eval_time_end_step must be non-negative, got {end}")
+    if end is not None and start >= end:
+        raise ValueError(f"eval_time_start_step ({start}) must be less than eval_time_end_step ({end})")
     current_stable = current_gpu_util_values[start:end]
     golden_stable = golden_gpu_util_values[start:end]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/performance/utils/evaluate.py` around lines 328 - 333, Validate and
clamp the computed start and end before slicing current_gpu_util_values and
golden_gpu_util_values: ensure start and end are integers >= 0, clamp them to
the valid range (0 .. len(steps)), and check start < end (raise a ValueError or
return a clear error) so you don't produce negative-index slices or empty
ranges; update the logic around config, start, end, steps,
current_gpu_util_values and golden_gpu_util_values to perform these checks and
fail fast with a clear message if inputs are invalid.